-
Notifications
You must be signed in to change notification settings - Fork 541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add arm64 support and Update Plex Download URLs to latest API #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments to facilitate discussion.
Thank you very much @gbooker for your feedback, I will work around all of the discussed points, then submit the following modifications during the next days! 👍 |
CONT_CONF_FILE="/version.txt" | ||
|
||
function addVarToConf { | ||
local variable="$1" | ||
local value="$2" | ||
if [ ! -z "${variable}" ]; then | ||
echo ${variable}=${value} >> ${CONT_CONF_FILE} | ||
fi | ||
} | ||
|
||
function readVarFromConf { | ||
local variable="$1" | ||
declare -n value=$2 | ||
if [ ! -z "${variable}" ]; then | ||
value="$(grep -w ${variable} ${CONT_CONF_FILE} | cut -d'=' -f2 | tail -n 1)" | ||
else | ||
value=NULL | ||
fi | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbooker I think there are better ways to write different variables to a "conf" file, but I wanted to keep things simple given that these variables are written once in this file (while building the image), then they are read only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the values are written once and in only one place, the function to add them isn't really needed (doesn't hurt though). I was expecting just a
echo "version=${TAG}" > /version.txt
echo "plex_build=${PLEX_BUILD}" >> /version.txt
echo "plex_distro=${PLEX_DISTRO}" >> /version.txt
but the above should work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly I was thinking to write the same lines at the beginning like you are suggesting. At least if at some point we need to have such as a function, we just have to modify the behaviour of this one
@gbooker I have made the changes we discussed last week to the corresponding files. I am waiting for your feedback, do not hesitate to suggest any enhancement you're thinking they are relevant :) Btw, I have tested this in the same environment from scratch, it's working as expected |
local variable="$1" | ||
declare -n value=$2 | ||
if [ ! -z "${variable}" ]; then | ||
value="$(grep -w ${variable} ${CONT_CONF_FILE} | cut -d'=' -f2 | tail -n 1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanations for reading value :
Expecting file should be like:
version=beta
plex-distro=debian
...
Value is : look for variable name (grep), then take the part after the '='
sign (cut), then take the last value if multiple (tail) (that should not happen but in case).
Overall this is looking good. I need to have a discussion with our build-team to see if we can make our CI build the ARM64 images as well. |
Ok nice, I am looking forward to see what's happening next, please let me know :) |
@maxlabuche @gbooker I would really be interested in this, as I want to have a setup with the raspberry pi 4. Let me know if there is anything I can help with. |
Hi @half2me , I am glad to see other people interested in supporting ARM architectures :) I have made some tests at home and it's working fine for me (with RPi 4 / 4GB, better with at least 2GB of RAM on arm64), so if you are interested in testing it, you can pull my image on my Docker registry before the integration in the official repository. (Instructions are exactly the same as the official one, and update checking is automatic each time you start the container) https://hub.docker.com/r/maxlbch/pms-docker-arm64 Enjoy! |
@maxlabuche I have the rpi4 4gb as well, so I will try your image. Thanks :) |
You're welcome :) |
@maxlabuche any chance you could build your image against amd64 arch and upload? (You can have multiple archs under the same tag) I'm trying to make my own helm chart for plex media server, and I would like to use your modifications on arm64 and amd64 as well. |
@half2me Yes that was my base idea to have multiple arch under the same tag, I just forgot to do that, I am working on it I'll let you know when it's pushed to the registry :) |
@maxlabuche a bit off topic, but if its of any help, I use github actions to build my docker images automatically on each push using |
@half2me Oh that's this kind of stuff I am looking for, indeed I was using buildx to build my images but for some reasons it failed for arm64, that's why I made it directly from the Raspberry, I will look around, thanks! |
I remember, here the thing is that the Dockerfile is not the same for amd64 and arm64, that's what I should consider |
Do you think it would be worth making it the same, and branching via an ARG? If most of it is similar... |
We need to look around before, for instance here is the muti-arch image : I will be working on your suggestion to be even more futureproof. |
@maxlabuche Exactly what I was looking for, thank you! |
Why this can't be merged? |
Dockerfile
Outdated
@@ -15,11 +18,12 @@ RUN \ | |||
xmlstarlet \ | |||
uuid-runtime \ | |||
unrar \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need unrar
and all of these programs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ipeacocks, actually I am not an expert of the Plex Core and features, so I can't answer you about that.. Maybe @gbooker can bring you more details about that.
I just added udev
because can be useful to access external storage like in my Raspberry project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit offtop. Why do you use 64 bit OS for Raspberry Pi. Raspberry advices to use 32-bits Raspbian even on 64-bits Pi 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use Pi 4 (RAM 4GB) with lots of containers on it (maria-db, postgre-db, web servers, python server, plex...) All of these is consuming lots of RAM and even more when several people are connected simultaneously. I am also using Ubuntu Server for ARM (no GUI), not Raspbian.
I haven't made deep benchmarks, but I can tell there is a significant gain in global performance and responsiveness using 64-bit with such a config. 32-bit architectures have per-process limits when it comes to use large amount of RAM.
Hope that's answering your question 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ipeacocks just like @maxlabuche I also 64bit. I believe in the future, this will be the only option, its just a matter of time. I'd like to make the switch as early as possible. Plus the performance gains :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really need
unrar
and all of these programs?
unrar
is needed for some of the third-party plugins (subtitles IIRC) but maybe it's not needed anymore. The rest of the above are definitively needed.
I just added
udev
because can be useful to access external storage like in my Raspberry project
udev
doesn't work as you would expect in containers. I'm skeptical of this and missed it in the first pass through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK @gbooker for udev
, we can skip it if not needed, I have also figured out other solutions. So what is missing to merge this PR? Do you have any concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experimentation, udev
didn't work inside the containers. We used to have it there but its lack of functionality prompted us to remove it.
We are still looking at getting our CI to build ARM images but that may take longer than expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gbooker , do not hesitate to add new comments.
I submitted my work, seems like CI team is still reviewing / testing it.. Do you have any news @gbooker ? Thanks, hope you guys are doing great. Stay safe |
It might be worth renaming the Dockerfile to |
@dekimsey You're right, but on Docker Hub 64-Bit ARM architectures are under the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting our CI to build ARM64 images is going to take longer so I'm going to target merging this without that. I think that udev
shouldn't be in the image since it doesn't actually work in containers but otherwise I'm fine with the changes in this PR.
@gbooker not sure what you use in your CI to build arm images, but I've had success using docker buildx to cross-compile for arm. If there is anything I can help with, let me know. |
@half2me Speaking of buildx, I'm trying to build using this setup (with my armhf addition) and buildx to make all the versions. But I'm not seeing how to tell buildx that it's one build but different Dockerfiles based on the architecture. Basically, it's trying to use Any ideas on how to do what I'm describing so that pms-docker:latest would work for all supported architectures? Or should we maybe consider unifying the Dockerfiles into a single one that builds correctly no matter what architecture you're on? |
FWIW, I rewrote my version to support x86, x86_64, aarch64, and armhf using a single Dockerfile so it's far more buildx friendly. |
@jayandcatchfire personally I like having a single Dockerfile whenever possible, but if there are too many differences between archs, you are better off with multiple Dockerfiles. You can simply tell docker buildx to use a specific Dockerfile for the build. If you give them all the same tag, they should be consolidated. |
@gbooker OK, that's done, I just removed @jayandcatchfire I like your work, for now I just wanted to add minor updates to this repo, that's why I didn't merge your PR to my branch. Also well suggested by @half2me , working on a multi-arch Dockerfile is definitely a good idea. Currently with the worldwide situation I can not put a lot of time on this project but I can help you if you need support implementing that (with buildx, scripting, etc.) Thank you all for your very interesting reviews! Do not hesitate to @ me in your comments if you need further help. Take care. |
@half2me Please tell me more about that. When I ran buildx on separate Dockerfiles serially, it replaced the latest tag on Docker Hub for me rather than adding to it. |
* Add arm64 support and Update Plex Download URLs to latest API (plexinc#48) Add arm64 support and Update Plex Download URLs to latest API * Create Dockerfile.armv7 Co-authored-by: Maxime Marmont <[email protected]> Co-authored-by: Jay Sherby <[email protected]>
(EDIT 1: Making my PR explanations more understandable)
Hi Plex Community,
I have a suggestion for the official Plex Docker Image, which is to make it available for different platforms other than Ubuntu/amd64.
Modification suggested by my branch:
Testing environment:
I have tested this with a Raspberry Pi 4 with this configuration:
Everything works fine, even updates.
Please consider merging my branch to this repo, I am available for any feedback!
Greetings